Adding an option for `--disable-nodegroup-eviction` when deleting a cluster #4659

AmitBenAmi · 2022-01-19T09:41:31Z

Description

When deleting a cluster that has pods with PDBs, I can not delete the whole cluster, since the nodes won't evict all of the workloads, causing the node groups draining step to fail.
Today, there is a flag when draining node groups of: --disable-eviction, which ignores PDB errors while draining the node groups.

I've added a flag: --disable-nodegroup-eviction, that will exist when deleting a cluster, and achieves the same behavior of the drain command with the --disable-eviction flag (it literally calls the drain function with the same value).

fixes #4416

Checklist

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the userdocs directory)
Manually tested
Made sure the title of the PR is a good description that can go into the release notes
(Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

pkg/actions/cluster/cluster.go

cPu1 · 2022-01-19T12:29:35Z

pkg/actions/cluster/delete.go

@@ -159,7 +163,7 @@ func checkForUndeletedStacks(stackManager manager.StackManager) error {
 	return nil
 }

-func drainAllNodegroups(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, stackManager manager.StackManager, clientSet kubernetes.Interface, allStacks []manager.NodeGroupStack) error {
+func drainAllNodeGroups(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, clientSet kubernetes.Interface, allStacks []manager.NodeGroupStack, disableEviction *bool, nodeGroupDrainer NodeGroupDrainer, vpcCniDeleter VpcCniDeleter) error {


There's no need to make disableEviction a pointer type.

Suggested change

func drainAllNodeGroups(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, clientSet kubernetes.Interface, allStacks []manager.NodeGroupStack, disableEviction *bool, nodeGroupDrainer NodeGroupDrainer, vpcCniDeleter VpcCniDeleter) error {

func drainAllNodeGroups(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, clientSet kubernetes.Interface, allStacks []manager.NodeGroupStack, disableEviction bool, nodeGroupDrainer NodeGroupDrainer, vpcCniDeleter VpcCniDeleter) error {

cPu1 · 2022-01-19T12:33:34Z

pkg/ctl/delete/cluster_test.go

+				})
+			})
+			_, err := cmd.execute()
+			Expect(err).To(Not(HaveOccurred()))


Nit:

Suggested change

Expect(err).To(Not(HaveOccurred()))

Expect(err).NotTo(HaveOccurred())

aclevername · 2022-01-19T12:42:55Z

Thanks for opening the PR @AmitBenAmi, descriptions makes sense 👌 will take a look. +10000 for the lovely tests 😄

cPu1 · 2022-01-19T12:49:39Z

pkg/actions/cluster/delete.go

+type NodeGroupDrainer interface {
+	Drain(nodeGroups []eks.KubeNodeGroup, plan bool, maxGracePeriod, nodeDrainWaitPeriod time.Duration, undo, disableEviction bool) error
+}
+type VpcCniDeleter func(clusterName string, ctl *eks.ClusterProvider, clientSet kubernetes.Interface)


We don't need to export this.

Suggested change

type VpcCniDeleter func(clusterName string, ctl *eks.ClusterProvider, clientSet kubernetes.Interface)

type vpcCNIDeleter func(clusterName string, ctl *eks.ClusterProvider, clientSet kubernetes.Interface)

pkg/actions/cluster/delete_test.go

pkg/actions/cluster/owned_test.go

pkg/actions/cluster/unowned_test.go

aclevername · 2022-01-19T16:55:10Z

pkg/actions/cluster/delete_test.go

@@ -0,0 +1,127 @@
+package cluster


mhmm does this test file actually get run when you run ginkgo -r pkg/action/cluster/? I would of thought it needs to be in cluster_test suite to register. But I get that it needs to have access to the private function drainAllNodeGroups.

I'd prefer if we made drainAllNodeGroups public, and moved this file into cluster_test package.

what do you think 😄 ?

This was my initial problem, which caused me to move it to a different package.
I didn't want to make the drainAllNodeGroups public so I moved it to be in the same package.
Do you think it is a good thing to make it publicly?

Also, when I run ginkgo -r ./pkg/action/cluster it does run these tests

Do you think it is a good thing to make it publicly?

I prefer it to testing private functions by including the test package in the code package. But some folks might disagree 😄

What do you mean by including the test package in the code package?

He means, that he prefers making the function public, and then testing it, rather than having the test code in the same package as the code it self, aka, the test code is in cluster package rather than cluster_test package which makes it a separate package. The benefit of having cluster_test is that you test the public API of the package. If a private function can't be tested through that, it means that it would need a bit of refactoring maybe or it's not working encapsulating, etc.

This isn't a dogma of course. :) It's a suggestion. Try and test private functions through utilising the public function. If it's hard to test, maybe there are certain conditions which are difficult to achieve in the first place?

well summarised @Skarlso . Does that make sense @AmitBenAmi ? Its not a blocker on merging as its a personal preference

@aclevername @Skarlso I changed it to fit this desire.
Basically, I also agree, and I wasn't happy with setting the test file inside the same package.
I synthetically exported the private function inside the export_test.go file, so I could test it outside of the cluster package, and now the tests file is in its own cluster_test package

pkg/actions/cluster/delete_test.go

aclevername · 2022-01-24T13:03:52Z

pkg/actions/cluster/owned.go

+		nodeGroupManager := c.newNodeGroupManager(c.cfg, c.ctl, clientSet)
+		if err := drainAllNodeGroups(c.cfg, c.ctl, clientSet, allStacks, disableNodegroupEviction, nodeGroupManager, attemptVpcCniDeletion); err != nil {
+			if force {
+				logger.Warning("error occurred during nodegroups draining")


Suggested change

logger.Warning("error occurred during nodegroups draining")

logger.Warning("error %q occurred during nodegroups draining, force=true so proceeding with deletion", err.Error())

probably find better wording 😆 but the improvement makes sense 😄

aclevername · 2022-01-24T13:05:45Z

pkg/actions/cluster/owned_test.go

+				})
+
+				err := c.Delete(time.Microsecond, false, false, false)
+				Expect(err).To(HaveOccurred())


can we improve this by changing to

Suggested change

Expect(err).To(HaveOccurred())

Expect(err).To(MatchError("whatever error I expected"))

or if we don't know the full error: Expect(err).To(MatchError(ContainSubstring("whatever sub-error I expected")))

same in some of the other test 😄

Changed it to match the mocked exception message

Skarlso · 2022-01-25T08:52:15Z

pkg/actions/cluster/delete.go

-	nodeGroupManager := nodegroup.New(cfg, ctl, clientSet)
-	if err := nodeGroupManager.Drain(cmdutils.ToKubeNodeGroups(cfg), false, ctl.Provider.WaitTimeout(), 0, false, false); err != nil {
+
+	if err := nodeGroupDrainer.Drain(cmdutils.ToKubeNodeGroups(cfg), false, ctl.Provider.WaitTimeout(), 0, false, disableEviction); err != nil {


Not necessary, but Drain has now quite a lot of parameters. Would be nice to have them in a struct rather, where we can clearly read which 0 and which false, false belongs to which parameter. :)

Skarlso · 2022-01-25T08:58:22Z

pkg/actions/cluster/owned.go

+			if force {
+				logger.Warning("an error occurred during nodegroups draining, force=true so proceeding with deletion: %q", err.Error())
+			} else {
+				return err
+			}


Suggested change

if force {

logger.Warning("an error occurred during nodegroups draining, force=true so proceeding with deletion: %q", err.Error())

} else {

return err

}

if !force {

return err

}

logger.Warning("an error occurred during nodegroups draining, force=true so proceeding with deletion: %q", err.Error())

Just a preference. :) I rather skip elses to ease reading the code.

Skarlso · 2022-01-25T09:00:50Z

pkg/actions/cluster/unowned.go

+			if force {
+				logger.Warning("an error occurred during nodegroups draining, force=true so proceeding with deletion: %q", err.Error())
+			} else {
+				return err
+			}


Suggested change

if force {

logger.Warning("an error occurred during nodegroups draining, force=true so proceeding with deletion: %q", err.Error())

} else {

return err

}

if !force {

return err

}

logger.Warning("an error occurred during nodegroups draining, force=true so proceeding with deletion: %q", err.Error())

It's a bummer that this has to be repeated. :/ ¯\_(ツ)_/¯

Skarlso

Generally, this is fantastic work. :) I have like two nits, and the fact that it has to be duplicated for owned and unowned. I wished that wouldn't be the case, but nothing you can do about that right now. :)

Signed-off-by: Amit Ben Ami <[email protected]>

AmitBenAmi · 2022-01-30T16:37:39Z

Hey @aclevername ,
What are the next steps in order to merge this PR?

…er-with-force-flag

Himangini · 2022-01-31T11:16:45Z

Hey @aclevername , What are the next steps in order to merge this PR?

@AmitBenAmi we'll merge this as soon as the build is green :) thanks for all the hard work 🎉

…ce-flag' of github.com:AmitBenAmi/eksctl into disabling-nodes-eviction-when-deleting-cluster-with-force-flag

AmitBenAmi · 2022-01-31T12:49:17Z

@Himangini Thanks, I pushed another commit to fix the lint errors

…er-with-force-flag

Skarlso · 2022-01-31T14:11:33Z

Finally. :D :D Well done! Nice job. Thank you for your contribution @AmitBenAmi.

AmitBenAmi · 2022-01-31T14:18:37Z

@Skarlso Thanks for the help and feedback, it was a pleasure 😄

cPu1 added the kind/feature New feature or request label Jan 19, 2022

aclevername requested review from aclevername, nikimanoledaki, cPu1, Himangini and Skarlso and removed request for aclevername January 19, 2022 12:26